Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

fix: DIA-754: [FE] Search option does not reflect actual queries state #279

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

yyassi-heartex
Copy link
Contributor

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

api requests can now check if request is already in flight - if already in flight cancel first promise and use the new one. this prevents a race condition where second request resolves before the first causing the first to override changes applied by the second (where second one has the correct outcome due to having more up to date params)

Comment on lines +548 to +549
const controller = new AbortController();
const signal = controller.signal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this apiCall the same one as the DM/LSF sdk uses all over? I only ask because that one already employs a AbortController concept. This does look like a different set of code though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i saw abort controller in LSP, not in DM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this call get made from? I realize there are differing calls now that I look, some are from the useAPI hook which underlying uses an api client, and those I believe have a similar but different method and signature to this one. Overall I think this change should be fine, the only concern I have is if this conflicts and breaks all the existing calls and hooks we have that use abort controllers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid concern! from within datamanager apiCall is used in many places, annotations, tasks, tab, comments, etc basically we can safely assume any where we call a request to the api this is the function that handles it - i just saw that NotificationsProvider in LSP also calls it.

i did have that in mind though which is why i updated const requestBody = { signal, ...(apiTransform?.body?.(body) ?? body) }; like this, with the intent that signal can be overridden by any one using apiCall with their own signal

Copy link
Contributor

@bmartel bmartel Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarity, and the foresight to handle it that way. Ok, I feel much better now about this given it has intent to be supportive of the various use cases. Last question, when an abort signal is sent, it produces an error which will be thrown and inevitably caught in the error handler (which produces a modal with the error) if not filtered out by the caller using the call options. Will this produce error modals in cases where the abort signal rightfully is working to ensure consistency on calls made?

Copy link
Contributor Author

@yyassi-heartex yyassi-heartex Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no i dont expect any modals to be generated here - i think it should be caught specifically by the calling function as it may need to be handled in different ways - the triggering function just needs this to cancel silently since there is a new request that we need to pay attention to and we're aiming to prevent race conditions that come up from following requests resolving after requests that are already in flight

Copy link
Contributor

@bmartel bmartel Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it should be caught specifically by the calling function as it may need to be handled in different ways

I agree, but my question is more to the effect of this change on all existing api calls, and whether this might end up producing modals for all unhandled cases where there are these abort signals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, alot of testing will be required to double check but from my testing, i didnt see any of these modals come up. i'll take a deeper look and mark the LSE pr ready for review to do some checking on FB aswell

Copy link
Contributor

@bmartel bmartel Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we might want to do something similar to what you have already here, and append to the errorFilter a check to remove the abort signal calls in every case there ends up being a signal (which is every time now). Because the one thing we do know, is regardless if someone supplied an errorFilter or not, we never want the abort signal to throw. In fact this would alleviate a lot of cases of having to remember to provide this to the api calls in general, because it isn't an error to be caught ever upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only time cancelation trigger was on tabUpdate and tasks for both projects and datasets, this is a good thing since both these requests can easily be hammered just by repeatedly toggling a column. the most recent changes should alleviate this significantly and with the response now including isCanceled we can safely let the calling function deal with it (ie prevent continuing to the next step)

@yyassi-heartex yyassi-heartex merged commit 34d39c2 into master Dec 6, 2023
5 checks passed
@yyassi-heartex yyassi-heartex deleted the fb-dia-754/apicall-cancel branch December 6, 2023 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants